Skip to content

Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures #1758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures #1758

wants to merge 24 commits into from

Conversation

madhav-madhusoodanan
Copy link

@madhav-madhusoodanan madhav-madhusoodanan commented Mar 24, 2025 •
edited
Loading

Updates so far

  1. Moved the ARM-specific testing to the arm module, exposing only a test function at src/main.rs
  2. Created a SupportedArchitectureTest trait that has been implemented for arm and should be implemented for other architectures too. Allows us to expose functionality like building C/Rust test files and comparing outputs.
  3. Moved common functionality (CLI parsing, create files, compilations, compare outputs) into the common module for reuse by other architectures too.
  4. Implemented a match block for selection of architectures using the target CLI variable.

Reasoning

  1. The Intrinsic type may be specific to architectures, hence the implementation of a trait helps us to abstract the usage of such data types within the architecture-specific module

New architecture of intrinsic-test

IMG_9057

@rustbot
Copy link
Collaborator

rustbot commented Mar 24, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 2 times, most recently from 9900e81 to 546554d Compare March 24, 2025 19:10
@Amanieu
Copy link
Member

Amanieu commented Mar 25, 2025

The module should be called arm rather than aarch.

@Amanieu
Copy link
Member

Amanieu commented Mar 25, 2025

@JamieCunliffe feel free to have a look at this since you are the original author of intrinsic-test. This is part of a GSoC project to extend intrinsic-test to other architectures.

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 6 times, most recently from 5dce230 to 7ff8497 Compare March 26, 2025 15:32
@madhav-madhusoodanan
Copy link
Author

madhav-madhusoodanan commented Mar 26, 2025 •
edited
Loading

Are we using the --a32 flag in the CLI of intrinsic-test anywhere? I was unable to find its usage.
Would it be alright if I remove it, or is there some reasoning that I may be missing?

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 2 times, most recently from a12b4a4 to ff10311 Compare March 27, 2025 15:49
@Amanieu
Copy link
Member

Amanieu commented Mar 27, 2025

--a32 is not currently tested in CI, but it is used for 32-bit ARM targets (arm* and thumb*). Some ARM intrinsics are only available on 64-bit ARM targets and this flag skips those.

@madhav-madhusoodanan madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 9 times, most recently from 6e8851b to a2ce02c Compare March 27, 2025 19:19
@madhav-madhusoodanan madhav-madhusoodanan changed the title [DRAFT] Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures Mar 27, 2025
@madhav-madhusoodanan madhav-madhusoodanan marked this pull request as ready for review March 27, 2025 19:29
@madhav-madhusoodanan madhav-madhusoodanan marked this pull request as draft March 28, 2025 08:51
@madhav-madhusoodanan madhav-madhusoodanan changed the title Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures [DRAFT] Re-organize intrinsic-test to enable seamless addition of behaviour testing for more architectures Mar 28, 2025
@madhav-madhusoodanan
Feat: Moved majority of the code to arm module.
bf7aee7 
Reasoning:
1. Majority of code assumes the usage of `Intrinsic` and related types, which is derived from the JSON structure of the ARM intrinsics JSON source file
2. Further commits will start with extracting common parts of the code (eg: Create C/Rust file, Build C/Rust file, etc)
@madhav-madhusoodanan
Chore: Added SupportedArchitectureTest trait which must be implemen…
4c25aa7 
…ted for different architectures.

Next steps:
Move the existing ARM-specific implementation into one that fits well with this trait.
@madhav-madhusoodanan
chore: Added ProcessedCli to extract the logic to pre-process CLI s…
5b9f0e5 
…truct args
@madhav-madhusoodanan
chore: separated common logic within file creations, compile_c, compi…
bf7812c 
…le_rust and compare_outputs
@madhav-madhusoodanan
chore: code consolidation
7118074
@madhav-madhusoodanan
chore: added match block in src/main.rs
0b6d424
@madhav-madhusoodanan
fixed too many files open issue
b215673
@madhav-madhusoodanan
maintaining special list of targets which need different execution co…
8b1a734 
…mmand
@madhav-madhusoodanan
rename struct for naming consistency
18e49d6
@madhav-madhusoodanan
test commit to check if load_Values_c can be dissociated from targe…
83110d3 
…t logic
@madhav-madhusoodanan
added target field within IntrinsicType to perform target level che…
0d4be1c 
…cking cleanly
@madhav-madhusoodanan
Updated Argument::from_c to remove ArgPrep specific argument
02a69bd
@madhav-madhusoodanan
introduced generic types and code refactor
a4b46a7
@madhav-madhusoodanan
Added a macro to simplify <Arch>IntrinsicType definitions
e570aad
@madhav-madhusoodanan
renamed a64_only data member in Intrinsic to arch_tags
a668076
@madhav-madhusoodanan
Removed aarch64-be specific execution command for rust test files
f1b2931
@madhav-madhusoodanan
moved the C compilation commands into a struct for easier handling
fb84942
@madhav-madhusoodanan
Added dynamic dispatch for easier management of `<arch>ArchitectureTe…
757dd74 
…st` structs
@madhav-madhusoodanan
code cleanup
4935acb
@madhav-madhusoodanan
chore: file renaming
c1d2d77
@madhav-madhusoodanan
feat: made constraint common
90aff41
@madhav-madhusoodanan
fix: aarch64_be issues wthin compilation
941637c
@madhav-madhusoodanan
moved more code generation functionality to common
a4f084e
@madhav-madhusoodanan madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch 3 times, most recently from df2c75d to 5d360a0 Compare May 21, 2025 09:23
@madhav-madhusoodanan madhav-madhusoodanan force-pushed the restructure-intrinsic-test-crate branch from 5d360a0 to 66a88fe Compare May 21, 2025 09:45
@madhav-madhusoodanan
Copy link
Author

Okay, I'm more or less done with resolving minor issues now.
Is this PR good enough to be merged? @adamgemmell @Amanieu ?

madhav-madhusoodanan
madhav-madhusoodanan commented May 21, 2025
View reviewed changes
crates/intrinsic-test/src/common/write_file.rs
Comment on lines +31 to +37
pub fn write_rust_testfiles<T: IntrinsicTypeDefinition>(
intrinsics: Vec<&dyn IntrinsicDefinition<T>>,
rust_target: &str,
notice: &str,
definitions: &str,
cfg: &str,
) -> Vec<String> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One change I made in the last commit was adding the "definitions" argument so that we could accommodate the f16 formatting related changes made in this commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamgemmell adamgemmell adamgemmell left review comments

@Amanieu Amanieu Awaiting requested review from Amanieu

Assignees

@Amanieu Amanieu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@madhav-madhusoodanan @rustbot @Amanieu @adamgemmell